Skip to content

feat(linalg): make SVD sweep budget configurable - #235

Open
mubasharameen485-cloud wants to merge 3 commits into
kmolan:mainfrom
mubasharameen485-cloud:feat/156-svd-configurable-sweeps
Open

feat(linalg): make SVD sweep budget configurable#235
mubasharameen485-cloud wants to merge 3 commits into
kmolan:mainfrom
mubasharameen485-cloud:feat/156-svd-configurable-sweeps

Conversation

@mubasharameen485-cloud

Copy link
Copy Markdown
Contributor

Summary

Adds SvdSettings with a configurable max_sweeps field (default 60,
matching existing behaviour). Matrix::svd() now delegates to a new
Matrix::svd_with_settings() method.

Test plan

  • cargo test -p multicalc — all existing tests pass unchanged
  • Confirmed default (60 sweeps) matches previous hardcoded behaviour

Fixes #156

Add SvdSettings with a configurable max_sweeps field, defaulting
to 60 to preserve existing behaviour. Matrix::svd() now delegates
to Matrix::svd_with_settings() with the default settings.

Fixes kmolan#156
@rtmongold

rtmongold commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Please drop d8a6370 and 5b7fbaa — that work already landed in #172 (same get_3d / get_3d_custom + helix test). Rebase onto current main so this PR is only the SVD change. Note: on main those APIs were later renamed in #199 (flux_integral_3d / …), so these commits will also conflict/stale.

@rtmongold

Copy link
Copy Markdown
Collaborator

The SvdSettings / svd_with_settings direction looks right for #156, but svd.rs needs a clean pass before merge:

  • There’s an unfinished placeholder left in the docs ([...existing doc comments yahan rakho jaise the...]), and the svd rustdoc got duplicated/broken when the new methods were added.
  • Formatting/indentation around svd and svd_with_settings is messed up (lots of blank lines, methods not indented with the rest of the impl). Please tidy / rustfmt.
  • Rebase onto current main — svd there already uses self.is_finite(); keep that instead of the old nested loop.
  • Please pub use SvdSettings next to Svd, and add a small test that a custom max_sweeps actually changes behaviour.
  • Please add a CHANGELOG.md entry under [Unreleased] → ### Added for the configurable SVD sweep budget / SvdSettings.

Happy to re-review once it’s just the SVD change on top of main.

@kmolan

kmolan commented Jul 29, 2026

Copy link
Copy Markdown
Owner

@mubasharameen485-cloud please focus on merging this issue first, do not begin new PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make the SVD sweep budget configurable

3 participants